Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(x/gov)!: remove Accounts.String() #19850

Merged
merged 7 commits into from
Mar 26, 2024

Conversation

JulianToledano
Copy link
Contributor

@JulianToledano JulianToledano commented Mar 25, 2024

Description

ref:
#13140
#7448


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features
    • Introduced enhancements in handling and converting addresses across various functions and operations for improved compatibility and error reporting.
  • Bug Fixes
    • Adjusted argument types and parameter handling in functions such as NewDeposit, NewMsgDeposit, NewMsgVote, among others, to ensure accurate processing.
  • Refactor
    • Removed obsolete Accounts String method to streamline functionality.
    • Updated NewProposal function to accept proposer parameter as a string for consistency.
  • Tests
    • Enhanced testing by incorporating address conversion using codectestutil in msgs_test.go.

@JulianToledano JulianToledano requested a review from a team as a code owner March 25, 2024 11:27
Copy link
Contributor

coderabbitai bot commented Mar 25, 2024

Walkthrough

Walkthrough

This update focuses on enhancing the address handling within the Cosmos SDK's governance module by removing outdated methods and incorporating address encoding adjustments. It includes changes to function arguments, the introduction of address codecs, and updates to the handling of addresses in proposal submissions and operations. These modifications aim to improve modularity and reduce dependency on global configurations.

Changes

Files Change Summary
x/gov/.../CHANGELOG.md Removed Accounts String method, adjusted argument types, and updated appmodule.Environment.
x/gov/client/cli/prompt.go, prompt_test.go Added address codec parameter to Prompt and PromptMetadata, updated imports.
x/gov/keeper/deposit.go, proposal.go Added address conversion in deposit and proposal handling.
x/gov/simulation/operations.go Modified governance account address handling in simulation operations.
x/gov/types/v1/proposal.go Changed NewProposal function to accept string type for proposer.
x/gov/types/v1beta1/msgs_test.go Updated tests to handle address conversion using codectestutil.

Related issues

  • EPIC: remove global bech32 #13140: This PR addresses objectives related to removing global bech32 prefixes and improving address handling by implementing address codec usage in various functions. The changes align with the issue's goals of eliminating globals and enhancing modularity within the Cosmos SDK.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

@JulianToledano
Copy link
Contributor Author

A refactor may be needed to get rid of this :/ otherwise HasValidateBasic may be broken.

func (c MsgExecLegacyContent) ValidateBasic() error {
_, err := sdk.AccAddressFromBech32(c.Authority)
if err != nil {
return err
}
return nil
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 10

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between adc3432 and 0a1e101.
Files selected for processing (37)
  • tests/integration/gov/abci_test.go (7 hunks)
  • tests/sims/gov/operations_test.go (5 hunks)
  • x/gov/CHANGELOG.md (1 hunks)
  • x/gov/client/cli/prompt.go (7 hunks)
  • x/gov/client/cli/prompt_test.go (3 hunks)
  • x/gov/client/cli/tx.go (3 hunks)
  • x/gov/client/cli/tx_test.go (14 hunks)
  • x/gov/client/cli/util.go (1 hunks)
  • x/gov/client/cli/util_test.go (13 hunks)
  • x/gov/client/utils/query.go (1 hunks)
  • x/gov/client/utils/query_test.go (6 hunks)
  • x/gov/depinject.go (2 hunks)
  • x/gov/keeper/common_test.go (7 hunks)
  • x/gov/keeper/deposit.go (2 hunks)
  • x/gov/keeper/deposit_test.go (9 hunks)
  • x/gov/keeper/grpc_query_test.go (41 hunks)
  • x/gov/keeper/keeper_test.go (1 hunks)
  • x/gov/keeper/msg_server_test.go (56 hunks)
  • x/gov/keeper/proposal.go (2 hunks)
  • x/gov/keeper/proposal_test.go (10 hunks)
  • x/gov/keeper/tally_test.go (17 hunks)
  • x/gov/keeper/vote.go (2 hunks)
  • x/gov/keeper/vote_test.go (5 hunks)
  • x/gov/module.go (1 hunks)
  • x/gov/simulation/operations.go (7 hunks)
  • x/gov/types/v1/deposit.go (1 hunks)
  • x/gov/types/v1/msgs.go (1 hunks)
  • x/gov/types/v1/msgs_test.go (4 hunks)
  • x/gov/types/v1/params.go (1 hunks)
  • x/gov/types/v1/proposal.go (2 hunks)
  • x/gov/types/v1/proposals_test.go (3 hunks)
  • x/gov/types/v1/vote.go (2 hunks)
  • x/gov/types/v1beta1/deposit.go (1 hunks)
  • x/gov/types/v1beta1/msgs.go (3 hunks)
  • x/gov/types/v1beta1/msgs_test.go (3 hunks)
  • x/group/client/cli/prompt.go (4 hunks)
  • x/params/client/cli/tx.go (1 hunks)
Additional comments: 151
x/gov/types/v1/deposit.go (1)
  • 10-11: The change from sdk.AccAddress to string for the depositor parameter in NewDeposit aligns with the PR's objectives and should simplify address handling. Ensure that address conversion from sdk.AccAddress to string is correctly handled throughout the codebase to maintain address formatting and validation integrity.
x/gov/types/v1beta1/deposit.go (1)
  • 10-11: Identical to the change in v1/deposit.go, transitioning the depositor parameter from sdk.AccAddress to string in NewDeposit is a positive step towards simplifying address handling. Ensure thorough verification of address conversion handling throughout the codebase.
x/gov/types/v1beta1/msgs_test.go (1)
  • 4-4: The addition of codectestutil for address conversion in test cases is appropriate and aligns with the changes in the codebase.
x/gov/client/cli/prompt_test.go (1)
  • 12-12: The inclusion of codectestutil for updated address handling in test cases is a good practice, ensuring that tests align with the codebase changes.
x/params/client/cli/tx.go (1)
  • 70-73: The modification to retrieve and handle the from address as a string using clientCtx.AddressCodec.BytesToString(clientCtx.GetFromAddress()) is consistent with the PR's goal of standardizing address handling. Ensure that this change does not introduce any issues with address formatting or validation.
x/gov/types/v1beta1/msgs.go (1)
  • 27-30: The modifications to function signatures, changing parameters from sdk.AccAddress to string, are consistent with the PR's objectives to standardize address handling. Ensure that these changes are correctly implemented across the codebase and do not introduce issues with address formatting or validation.

Also applies to: 57-58, 82-83, 87-88, 92-93

x/gov/types/v1/msgs_test.go (1)
  • 13-13: The addition of codectestutil for address conversion in test cases is appropriate and aligns with the changes in the codebase.
x/gov/keeper/vote.go (2)
  • 71-74: The conversion from sdk.AccAddress to a string representation using k.authKeeper.AddressCodec().BytesToString(voterAddr) is a key part of the refactor. Ensure that all instances where voterAddr was previously used are now correctly using voterStrAddr.
  • 87-87: Ensure that the event attributes are correctly using the string representation of the voter address. This is crucial for maintaining consistency in how addresses are represented in events.
x/gov/depinject.go (2)
  • 79-82: The conversion of the authority address to a string and the subsequent error handling using a panic is a critical change. While using panic for unrecoverable errors is acceptable, ensure that this conversion error is indeed a scenario where the application cannot recover. In most cases, it's preferable to handle errors gracefully.

Consider if there's a more graceful way to handle this error that doesn't involve panicking, especially if this operation could fail under normal circumstances.

  • 92-92: The usage of the converted authority address (authorityAddr) in the NewKeeper function call aligns with the refactor's goal to handle addresses as strings. This ensures consistency in how addresses are managed within the module.
x/gov/types/v1/proposal.go (1)
  • 23-29: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [26-49]

The update to the NewProposal function to accept a proposer parameter of type string and the subsequent assignment to the Proposer field align with the refactor's goal to handle addresses as strings. This change ensures consistency in how addresses are managed within the governance module.

x/gov/types/v1/vote.go (1)
  • 26-27: The update to the NewVote function to accept a voter parameter of type string is consistent with the refactor's goal to handle addresses as strings. This change ensures consistency in how addresses are managed within the governance module.
x/gov/types/v1/msgs.go (3)
  • 90-91: The update to the NewMsgDeposit function to accept a depositor parameter of type string aligns with the refactor's goal to handle addresses as strings. This change ensures consistency in how addresses are managed within the governance module.
  • 95-96: The update to the NewMsgVote function to accept a voter parameter of type string is consistent with the refactor's goal to handle addresses as strings. This change ensures consistency in how addresses are managed within the governance module.
  • 100-101: The update to the NewMsgVoteWeighted function to accept a voter parameter of type string aligns with the refactor's goal to handle addresses as strings. This change ensures consistency in how addresses are managed within the governance module.
x/gov/client/utils/query_test.go (5)
  • 58-59: The introduction of cdcOpts for codec options in the TestGetPaginatedVotes function is a good practice, ensuring that codec configurations are explicitly defined and used throughout the test.
  • 69-70: Converting sdk.AccAddress instances to strings before being used in message creation functions is consistent with the refactor's goal to handle addresses as strings. This ensures that the test aligns with the updated module behavior.
  • 73-74: Similar to the previous comment, converting another sdk.AccAddress instance to a string before use is correctly implemented and aligns with the refactor's objectives.
  • 76-78: The use of string representations for addresses in message creation functions within the test is correctly implemented. This change ensures that the test reflects the updated behavior of the governance module.
  • 81-82: Again, the use of string representations for addresses in message creation functions is correctly implemented, ensuring consistency in how addresses are managed within the test.
x/group/client/cli/prompt.go (4)
  • 12-12: The addition of the address import from "cosmossdk.io/core/address" is necessary for the changes made in this file, specifically for using the address.Codec interface. This ensures that address encoding and decoding are handled consistently across the module.
  • 35-37: The modification to the Prompt method to accept an additional addressCodec parameter and its use in calling govcli.PromptMetadata is correctly implemented. This change aligns with the refactor's goal to handle addresses as strings and ensures that the Prompt method can correctly prompt for metadata using the provided addressCodec.
  • 75-75: The use of the addressCodec parameter in calling govcli.Prompt for setting the proposal message is correctly implemented. This ensures that the Prompt method can correctly handle message prompting with the provided addressCodec, aligning with the refactor's objectives.
  • 146-146: The update to the call to the Prompt method in NewCmdDraftProposal to pass clientCtx.AddressCodec as the addressCodec parameter is correctly implemented. This ensures that the Prompt method receives the correct addressCodec for its operations, aligning with the refactor's goal to handle addresses as strings.
x/gov/client/utils/query.go (1)
  • 123-126: The conversion of params.Voter from sdk.AccAddress to a string using clientCtx.AddressCodec.BytesToString(params.Voter) is a necessary change to align with the PR's objective of handling addresses as strings. This ensures consistency in address handling across the module.
x/gov/client/cli/util.go (1)
  • 204-208: The update to convert the address using clientCtx.AddressCodec.BytesToString(clientCtx.GetFromAddress()) before passing it to ReadGovPropCmdFlags is in line with the PR's goal of standardizing address handling as strings. This change enhances consistency and interoperability within the module.
x/gov/keeper/keeper_test.go (1)
  • 75-78: The conversion of the governance account address to a string using suite.acctKeeper.AddressCodec().BytesToString(govAcct) before passing it to NewLegacyMsgServerImpl is a good practice. It ensures that the address is handled in a consistent format throughout the tests, aligning with the PR's objectives.
x/gov/module.go (1)
  • 126-130: Obtaining the module address as a string using am.accountKeeper.AddressCodec().BytesToString(am.accountKeeper.GetModuleAddress(govtypes.ModuleName)) before registering the message server in RegisterServices is crucial for consistency in address handling. This change aligns with the PR's goal and ensures that addresses are managed uniformly across the module.
x/gov/client/cli/tx.go (3)
  • 141-146: Converting the proposer's address to a string in NewMsgSubmitProposal using clientCtx.AddressCodec.BytesToString(clientCtx.GetFromAddress()) before creating a proposal message is aligned with the PR's objectives. This ensures that addresses are handled consistently as strings.
  • 211-216: Similarly, in NewCmdSubmitLegacyProposal, converting the proposer's address to a string before creating a legacy proposal message maintains consistency in address handling across the module.
  • 260-263: In NewCmdWeightedVote, converting the voter's address to a string before submitting a weighted vote ensures uniform address handling, aligning with the PR's goal of standardizing address management.
x/gov/keeper/vote_test.go (1)
  • 25-29: Converting addresses to strings using authKeeper.AddressCodec().BytesToString in the voting tests enhances readability and maintainability by using string representations for address comparisons. This change aligns with the PR's objectives of standardizing address handling.
x/gov/keeper/common_test.go (2)
  • 83-94: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [86-100]

The function mockDefaultExpectations now returns an error, which is a good practice for error handling. However, ensure that all callers of this function properly handle the returned error. Failing to do so could lead to ignored errors that might affect the reliability of the tests.

Verification successful

The verification process confirms that the error returned by mockDefaultExpectations is properly handled by its caller within the x/gov/keeper/common_test.go file. This aligns with the best practices for error handling, especially in test environments, ensuring the reliability of the tests.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for calls to mockDefaultExpectations and ensure error handling is implemented.
rg --type go 'mockDefaultExpectations\(' --context 2

Length of output: 591

* 184-227: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [171-226]

The function trackMockBalances has been updated to handle errors more robustly, particularly with address conversions. This is a positive change for error handling. However, ensure that the error messages provided are clear and helpful for debugging. For example, when an error occurs during address conversion, it might be useful to include the problematic address in the error message.

- return err
+ return fmt.Errorf("failed to convert address: %v, error: %w", sender, err)

Apply this pattern to all similar error returns within trackMockBalances.

x/gov/client/cli/prompt.go (3)
  • 96-106: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [68-103]

The addition of addressCodec to the Prompt function and its usage for address conversion is a good practice for handling different address formats. However, ensure that the error handling for address conversion is consistent and provides clear feedback to the user. For example, when an error occurs during address conversion, it might be useful to include more context in the error message.

- return data, err
+ return data, fmt.Errorf("failed to convert authority address: %v, error: %w", types.ModuleName, err)

Apply this pattern to all similar error returns within the Prompt function.

  • 163-170: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [166-193]

The modifications to the proposalType method Prompt to include addressCodec and handle address encoding are aligned with the overall changes in this PR. However, ensure that the error messages are informative and guide the user on what went wrong, especially in cases where address conversion might fail.

- return nil, metadata, fmt.Errorf("failed to set proposal message: %w", err)
+ return nil, metadata, fmt.Errorf("failed to set proposal message due to address conversion error: %w", err)

This change provides more context in the error message, making it easier for users to understand the issue.

  • 217-219: The update to PromptMetadata to include addressCodec is consistent with the changes made in other parts of the file. However, consider adding error handling or validation for the address conversion within this function if applicable. If address conversion is not directly used in this function, ensure that any downstream functions that require addressCodec are properly handling potential errors.
x/gov/keeper/proposal.go (1)
  • 131-136: The conversion of the proposer's address to a string before creating a new proposal is correctly implemented. This change aligns with the PR's objective to handle addresses as strings. No issues found here.
x/gov/keeper/deposit.go (2)
  • 168-172: The conversion of the depositor's address to a string before creating a new deposit is correctly implemented. This change is consistent with the PR's objective to handle addresses as strings. No issues found here.
  • 254-257: The conversion of the pool module account address to a string before using it in operations is correctly implemented. This ensures consistency in address handling throughout the module. No issues found here.
x/gov/client/cli/tx_test.go (1)
  • 71-72: The introduction of val0StrAddr to store the string representation of val[0].Address and its subsequent use in function calls is correctly implemented. This change improves readability and consistency in address handling within the test cases.
x/gov/types/v1/params.go (1)
  • 263-263: The change to use addressCodec.StringToBytes for validating the ProposalCancelDest address aligns with the PR's objective to handle addresses as strings. This ensures consistency in address handling across the module. No issues found here.
x/gov/keeper/proposal_test.go (5)
  • 16-16: The addition of codectestutil import is appropriate for the new address handling logic introduced in the tests.
  • 126-136: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [129-142]

The conversion of addresses using AddressCodec().BytesToString and the setting of message-based parameters in TestSubmitProposal are significant changes aligning with the PR's objectives to handle addresses as strings.

  • 167-167: The test case ensuring an error is thrown when the signer is not the governance account is a good practice for validating proposal submission logic.
  • 184-203: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [187-306]

The TestCancelProposal function has been significantly refactored to accommodate string-based address handling and to test various scenarios of proposal cancellation. This includes proper error handling and the validation of expected error messages, which enhances the test coverage for proposal cancellation logic.

  • 333-335: The TestMigrateProposalMessages function correctly utilizes the codectestutil for address conversion, ensuring consistency with the new address handling approach. This test validates the migration logic for proposal messages, which is crucial for backward compatibility and data integrity.
x/gov/keeper/deposit_test.go (9)
  • 43-44: Tracking mock balances with trackMockBalances and ensuring no error occurs is a foundational setup step for the deposit tests, ensuring that the test environment is correctly initialized.
  • 56-59: The conversion of addresses using AddressCodec().BytesToString for TestAddrs is consistent with the PR's objectives to handle addresses as strings. This change is crucial for ensuring that the tests align with the updated logic in the governance module.
  • 88-88: Asserting the depositor's address as a string in the deposit tests is a direct consequence of the transition to string-based address handling. This change ensures that the tests reflect the updated logic in the governance module.
  • 101-101: Similar to previous comments, asserting the depositor's address as a string after a second deposit from the same address validates the consistent handling of addresses as strings throughout the deposit logic.
  • 113-113: The assertion of the depositor's address as a string after a deposit from a new address further confirms the adherence to string-based address handling in the governance module's deposit logic.
  • 136-138: The validation of depositor addresses as strings in the test for deposit iterator functionality is consistent with the overall transition to string-based address handling. This ensures that the deposit logic correctly handles addresses as strings in various contexts.
  • 222-223: The setup step for TestDepositAmount involving tracking mock balances and ensuring no error occurs is essential for initializing the test environment correctly. This setup is foundational for testing the deposit amount logic under various scenarios.
  • 419-426: The handling of the proposal cancellation destination address, including the conversion to a string and the conditional logic based on the test case, demonstrates careful consideration of different scenarios in the deposit logic. This includes handling normal addresses and special addresses like the community pool address.
  • 453-457: The conversion of TestAddrs[0] to a string and its use in ChargeDeposit aligns with the PR's objectives to handle addresses as strings. This change is crucial for ensuring that the deposit charging logic reflects the updated address handling approach.
tests/sims/gov/operations_test.go (4)
  • 210-211: The changes correctly handle the conversion of the account address to a string, including proper error handling. This aligns with the PR's objectives of transitioning address handling from sdk.AccAddress to string.
  • 263-263: The replacement of sdk.AccAddress with a string for the address parameter in v1.NewProposal aligns with the PR's objectives.
  • 307-307: The replacement of sdk.AccAddress with a string for the address parameter in v1.NewProposal is consistent with the PR's objectives.
  • 349-349: The replacement of sdk.AccAddress with a string for the address parameter in v1.NewProposal aligns with the PR's objectives.
x/gov/client/cli/util_test.go (5)
  • 24-24: The addition of the codectestutil import is necessary for address conversion in tests, aligning with the PR's objectives.
  • 151-153: The conversion of addresses to strings in TestParseSubmitProposal is consistent with the PR's objectives, ensuring the tests align with the updated address handling.
  • 213-214: The update of assertions to use addrStr instead of direct address usage in TestParseSubmitProposal is necessary and aligns with the PR's objectives.

Also applies to: 218-219, 223-223

  • 307-308: The addition of the fromAddrStr variable and its usage in tests is correctly adapting the tests to the updated address handling.
  • 341-341: The conversion of fromAddr to a string and its usage in MsgSubmitProposal struct initialization in TestReadGovPropFlags correctly reflects the updated address handling.

Also applies to: 556-556, 573-573, 623-623, 640-640, 656-656, 673-673

x/gov/simulation/operations.go (7)
  • 188-194: The conversion from sdk.AccAddress to string using ak.AddressCodec().BytesToString() is correctly implemented. However, it's important to ensure that the GetGovernanceAccount method always returns a valid account, as this could potentially return nil, leading to a panic when calling GetAddress().

Ensure that GetGovernanceAccount has proper error handling and never returns nil without handling it.

  • 253-260: The conversion from sdk.AccAddress to string for the account address in NewMsgSubmitProposal is correctly implemented. This change aligns with the PR's objective to handle addresses as strings. However, ensure that the error from BytesToString is handled appropriately everywhere it's used to prevent potential runtime panics.
  • 355-359: The conversion from sdk.AccAddress to string in SimulateMsgDeposit is correctly implemented. This is consistent with the PR's goal of transitioning to string-based address handling. It's crucial to ensure that all address conversions are handled consistently across the module to avoid errors.
  • 429-433: The conversion from sdk.AccAddress to string in operationSimulateMsgVote is correctly implemented. This change is part of the broader refactor to handle addresses as strings. Consistency in error handling for the address conversion is important to maintain throughout the module.
  • 496-500: The conversion from sdk.AccAddress to string in operationSimulateMsgVoteWeighted is correctly implemented. This aligns with the refactor's goal to handle addresses as strings. Ensure that the error handling for address conversion is consistent and robust across all instances.
  • 540-544: The conversion from sdk.AccAddress to string in SimulateMsgCancelProposal is correctly implemented. This change supports the PR's objective of transitioning to string-based address handling. It's essential to ensure that error handling for address conversion is consistent across the module.
  • 555-559: The conversion from sdk.AccAddress to string for the account address in NewMsgCancelProposal is correctly implemented. This is part of the broader effort to handle addresses as strings. Ensure that error handling for the address conversion is consistent and robust across all instances.
tests/integration/gov/abci_test.go (7)
  • 28-29: The conversion from sdk.AccAddress to string using suite.AccountKeeper.AddressCodec().BytesToString() is correctly implemented for test setup. This change is necessary for the tests to align with the updated address handling in the governance module. Ensure that all tests are updated accordingly to maintain consistency.
  • 56-57: The conversion from sdk.AccAddress to string for address handling in tests is correctly implemented. This change ensures that the tests are consistent with the module's updated logic for address handling. It's important to ensure that all tests follow this pattern for address conversion.
  • 200-202: The conversion from sdk.AccAddress to string in TestTickPassedDepositPeriod is correctly implemented. This change aligns with the module's transition to string-based address handling. Consistency in address handling across all tests is crucial for maintaining test reliability.
  • 301-303: The conversion from sdk.AccAddress to string in TestTickPassedVotingPeriod is correctly implemented. This ensures that the tests are in line with the governance module's updated address handling logic. Consistency in address conversion across tests is important for test accuracy.
  • 383-385: The conversion from sdk.AccAddress to string in TestProposalPassedEndblocker is correctly implemented. This change is part of ensuring that the tests reflect the module's transition to string-based address handling. It's important to ensure that all tests are updated to maintain consistency.
  • 445-447: The conversion from sdk.AccAddress to string in TestEndBlockerProposalHandlerFailed is correctly implemented. This ensures that the tests are consistent with the governance module's updated logic for address handling. Consistency in address handling across all tests is crucial for maintaining test reliability.
  • 545-547: The conversion from sdk.AccAddress to string in TestExpeditedProposal_PassAndConversionToRegular is correctly implemented. This change aligns with the module's transition to string-based address handling. It's important to ensure that all tests follow this pattern for address conversion.
x/gov/keeper/grpc_query_test.go (14)
  • 22-22: The conversion from sdk.AccAddress to string using AddressCodec().BytesToString is correctly implemented. However, it's not clear where govAcct is defined. Ensure govAcct is properly initialized before this conversion.
  • 60-60: The use of govAcctStr in NewLegacyContent indicates a shift towards using string representations for account addresses, aligning with the PR's objectives. This change is correctly applied.
  • 106-106: Similar to the previous comment, ensure govAcct is defined and initialized before its conversion to govAcctStr. This pattern is consistent across the file, indicating a systematic approach to handling account addresses as strings.
  • 197-197: The conversion of addrs[0] to addr0Str is correctly implemented. This change is part of the broader refactor to handle addresses as strings. Ensure that addrs is correctly populated with sdk.AccAddress instances before this line.
  • 223-223: The conversion of the governance account address to a string is correctly implemented. This change is consistent with the PR's objectives to handle addresses as strings. Ensure the governance account is correctly retrieved and not nil.
  • 287-287: Creating a new deposit with addr0Str as the depositor's address is correctly implemented, aligning with the refactor to use string representations for addresses. This change is part of the broader effort to standardize address handling.
  • 414-414: Again, ensure govAcct is defined and initialized before its conversion to govAcctStr. This repeated pattern across different test suites indicates a comprehensive approach to refactoring address handling.
  • 461-461: The conversion of addrs[0] to addr0Str is correctly implemented, following the PR's objectives. Ensure that addrs is correctly populated with sdk.AccAddress instances before this line.
  • 582-582: Similar to previous comments, ensure govAcct is defined and initialized before its conversion to govAcctStr. This consistency across test suites supports the PR's objectives.
  • 705-705: The conversion of addrs[0] to addr0Str is correctly implemented, aligning with the refactor to handle addresses as strings. Ensure that addrs is correctly populated with sdk.AccAddress instances before this line.
  • 1120-1120: Ensure govAcct is defined and initialized before its conversion to addr0Str. This pattern is consistent across the file, indicating a systematic approach to handling account addresses as strings.
  • 1226-1226: Similar to previous comments, ensure govAcct is defined and initialized before its conversion to addr0Str. This repeated pattern across different test suites indicates a comprehensive approach to refactoring address handling.
  • 1333-1333: The conversion of addrs[0] to addr0Str is correctly implemented, following the PR's objectives. Ensure that addrs is correctly populated with sdk.AccAddress instances before this line.
  • 1436-1436: Ensure govAcct is defined and initialized before its conversion to addr0Str. This consistency across test suites supports the PR's objectives.
x/gov/keeper/tally_test.go (16)
  • 148-154: The conversion from sdk.AccAddress to string using BytesToString is repeated multiple times across different test cases. Consider refactoring this into a helper function to improve code maintainability and reduce duplication.
  • 177-183: Similar to the previous comment, the address conversion logic is repeated. Refactoring into a helper function would enhance code readability and maintainability.
  • 207-213: Again, the repeated logic for address conversion is observed. Consolidating this logic into a single, reusable function would be beneficial for code cleanliness and maintainability.
  • 242-256: The repeated pattern of address conversion is evident here as well. Implementing a helper function for this purpose would streamline the code and improve maintainability.
  • 481-484: The use of require.NoError(t, err) inside a loop for converting validator addresses could potentially halt the test prematurely if an error occurs. While this is generally acceptable in tests to ensure setup correctness, ensure that this behavior is intentional and that the error handling aligns with the test's objectives.
  • 614-620: The address conversion logic is repeated here. Implementing a helper function to handle this conversion would make the tests cleaner and more maintainable.
  • 643-649: As with previous instances, the repeated address conversion logic could benefit from refactoring into a helper function to improve code maintainability.
  • 673-679: The pattern of repeating address conversion logic continues. A helper function would enhance code maintainability and readability.
  • 708-722: The repeated logic for address conversion is observed again. Refactoring this into a helper function would improve code maintainability.
  • 951-954: Similar to a previous comment, ensure that the use of require.NoError(t, err) inside a loop is intentional and aligns with the test's objectives. This could halt the test prematurely if an error occurs, which is generally acceptable for setup correctness but should be intentional.
  • 1044-1050: The repeated logic for address conversion is observed. Implementing a helper function for this conversion would improve code maintainability and readability.
  • 1248-1254: The address conversion logic is repeated here. A helper function to handle this conversion would make the tests cleaner and more maintainable.
  • 1277-1283: As with previous instances, the repeated address conversion logic could benefit from refactoring into a helper function to improve code maintainability.
  • 1307-1313: The pattern of repeating address conversion logic continues. A helper function would enhance code maintainability and readability.
  • 1342-1356: The repeated logic for address conversion is observed again. Refactoring this into a helper function would improve code maintainability.
  • 1501-1504: Ensure that the use of require.NoError(t, err) inside a loop for converting validator addresses is intentional and aligns with the test's objectives. This could halt the test prematurely if an error occurs, which is generally acceptable for setup correctness but should be intentional.
x/gov/keeper/msg_server_test.go (41)
  • 29-33: The conversion from sdk.AccAddress to string using suite.acctKeeper.AddressCodec().BytesToString(addrs[0]) and error handling with suite.Require().NoError(err) is correctly implemented. This change aligns with the PR's objective to handle addresses as strings.
  • 40-41: Using govStrAcct and proposerAddr as FromAddress and ToAddress in banktypes.MsgSend is consistent with the refactor to use string representations of addresses. This ensures compatibility with the updated address handling approach.
  • 70-70: The use of proposerAddr as a string in v1.NewMsgSubmitProposal is correct and aligns with the refactor's goals. This change is part of the broader effort to standardize address handling across the governance module.
  • 160-160: The error message "metadata too long" is clear and informative, providing direct feedback about the issue with the proposal's metadata length. This is a good practice for error messages, as it helps developers understand the cause of the error quickly.
  • 205-205: The test case "signer isn't gov account" correctly anticipates the scenario where a non-governance account attempts to submit a proposal. This is an important test case to ensure that only authorized accounts can perform governance actions.
  • 220-220: The test case "invalid msg handler" checks for the scenario where a proposal message is not recognized by the router. This is crucial for ensuring that only valid messages can be included in proposals, maintaining the integrity of the governance process.
  • 235-235: The error message "invalid deposit denom" is clear and directly addresses the issue with the deposited coin's denomination. This specificity in error messages is beneficial for debugging and understanding the constraints of governance actions.
  • 311-312: Converting the address to a string for the proposerAddr in the TestSubmitMultipleChoiceProposal function is consistent with the refactor's objectives. This ensures that the test aligns with the updated governance module's address handling.
  • 324-324: The test case for "empty options" in TestSubmitMultipleChoiceProposal correctly identifies a scenario where a multiple-choice proposal is submitted without any options. This test ensures that the governance module enforces the requirement for proposals to have at least two options.
  • 388-392: The conversion of both proposer and govAcct to string addresses (proposerAddr and govStrAcct) in TestMsgCancelProposal is correctly implemented. This consistency in handling addresses as strings is crucial for the refactor's success.
  • 396-397: Using string addresses in the banktypes.MsgSend within the TestMsgCancelProposal function aligns with the refactor's goals. This ensures that the test cases are compatible with the updated address handling in the governance module.
  • 404-404: The use of proposerAddr in v1.NewMsgSubmitProposal within the TestMsgCancelProposal function is correct. This change is part of the broader effort to standardize address handling across the governance module by using string representations.
  • 490-493: The conversion of addresses to strings in TestMsgVote for both proposer and the governance account is correctly implemented. This consistency in address handling is essential for aligning the test cases with the governance module's refactor.
  • 499-500: Using string addresses in the banktypes.MsgSend within the TestMsgVote function aligns with the refactor's goals. This ensures that the test cases are compatible with the updated address handling in the governance module.
  • 507-507: The use of proposerAddr in v1.NewMsgSubmitProposal within the TestMsgVote function is correct. This change is part of the broader effort to standardize address handling across the governance module by using string representations.
  • 612-618: The test case for "voter error" in TestMsgVote correctly simulates a scenario where an invalid voter address is used. This is important for ensuring that only valid addresses can participate in the voting process, maintaining the integrity of governance actions.
  • 670-675: The conversion of the governance account to a string address in TestMsgVoteWeighted is correctly implemented. This consistency in address handling is essential for aligning the test cases with the governance module's refactor.
  • 681-682: Using string addresses in the banktypes.MsgSend within the TestMsgVoteWeighted function aligns with the refactor's goals. This ensures that the test cases are compatible with the updated address handling in the governance module.
  • 689-689: The use of proposerAddr in v1.NewMsgSubmitProposal within the TestMsgVoteWeighted function is correct. This change is part of the broader effort to standardize address handling across the governance module by using string representations.
  • 965-966: The conversion of the governance account to a string address in TestMsgDeposit is correctly implemented. This consistency in address handling is essential for aligning the test cases with the governance module's refactor.
  • 976-977: Using string addresses in the banktypes.MsgSend within the TestMsgDeposit function aligns with the refactor's goals. This ensures that the test cases are compatible with the updated address handling in the governance module.
  • 984-984: The use of proposerAddr in v1.NewMsgSubmitProposal within the TestMsgDeposit function is correct. This change is part of the broader effort to standardize address handling across the governance module by using string representations.
  • 1054-1057: The conversion of depositor to a string address and its use in v1.NewMsgDeposit within the TestMsgDeposit function is correctly implemented. This aligns with the refactor's objective to handle addresses as strings, ensuring that the test cases are consistent with the updated governance module.
  • 1070-1071: The conversion of an address to a string using codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(...) in TestLegacyMsgSubmitProposal is correctly implemented. This ensures that the legacy tests are also aligned with the refactor's goals of standardizing address handling.
  • 1113-1113: The use of an empty address string in the test case "empty proposer" correctly simulates a scenario where a proposal is submitted without a valid proposer address. This test ensures that the governance module enforces the requirement for a valid proposer address in proposals.
  • 1182-1183: The conversion of the governance account to a string address in TestLegacyMsgVote is correctly implemented. This consistency in address handling is essential for aligning the test cases with the governance module's refactor.
  • 1193-1194: Using string addresses in the banktypes.MsgSend within the TestLegacyMsgVote function aligns with the refactor's goals. This ensures that the test cases are compatible with the updated address handling in the governance module.
  • 1201-1201: The use of proposerAddr in v1.NewMsgSubmitProposal within the TestLegacyMsgVote function is correct. This change is part of the broader effort to standardize address handling across the governance module by using string representations.
  • 1272-1278: The test case for "voter error" in TestLegacyMsgVote correctly simulates a scenario where an invalid voter address is used. This is important for ensuring that only valid addresses can participate in the voting process, maintaining the integrity of governance actions.
  • 1329-1334: The conversion of both the governance account and a proposer to string addresses in TestLegacyVoteWeighted is correctly implemented. This consistency in address handling is essential for aligning the test cases with the governance module's refactor.
  • 1340-1341: Using string addresses in the banktypes.MsgSend within the TestLegacyVoteWeighted function aligns with the refactor's goals. This ensures that the test cases are compatible with the updated address handling in the governance module.
  • 1348-1348: The use of proposerAddr in v1.NewMsgSubmitProposal within the TestLegacyVoteWeighted function is correct. This change is part of the broader effort to standardize address handling across the governance module by using string representations.
  • 1534-1539: The test case for "voter error" in TestLegacyVoteWeighted correctly simulates a scenario where an invalid voter address is used. This is important for ensuring that only valid addresses can participate in the voting process, maintaining the integrity of governance actions.
  • 1594-1599: The conversion of the governance account and a proposer to string addresses in TestLegacyMsgDeposit is correctly implemented. This consistency in address handling is essential for aligning the test cases with the governance module's refactor.
  • 1605-1606: Using string addresses in the banktypes.MsgSend within the TestLegacyMsgDeposit function aligns with the refactor's goals. This ensures that the test cases are compatible with the updated address handling in the governance module.
  • 1613-1613: The use of proposerAddr in v1.NewMsgSubmitProposal within the TestLegacyMsgDeposit function is correct. This change is part of the broader effort to standardize address handling across the governance module by using string representations.
  • 1665-1668: The conversion of depositor to a string address and its use in v1beta1.NewMsgDeposit within the TestLegacyMsgDeposit function is correctly implemented. This aligns with the refactor's objective to handle addresses as strings, ensuring that the test cases are consistent with the updated governance module.
  • 2244-2245: The conversion of an address to a string using suite.acctKeeper.AddressCodec().BytesToString(suite.addrs[0]) in TestMsgSudoExec is correctly implemented. This ensures that the test cases are aligned with the refactor's goals of standardizing address handling.
  • 2258-2258: The use of v1.NewMsgVote within TestMsgSudoExec and setting it as the sudo-ed message is a valid test case for ensuring that the governance module can perform actions on behalf of other accounts through sudo. This test case is important for verifying the functionality of the MsgSudoExec message.
  • 2218-2224: The setup for the TestSubmitProposal_InitialDeposit test cases, including the conversion of an address to a string and setting the governance parameters, is correctly implemented. This setup is crucial for testing the governance module's behavior under various conditions related to initial deposit requirements.
  • 2227-2227: The creation of a MsgSubmitProposal message within TestSubmitProposal_InitialDeposit using a string address for the proposer is correctly aligned with the refactor's objectives. This ensures that the test cases are consistent with the updated governance module's address handling.

x/gov/CHANGELOG.md Outdated Show resolved Hide resolved
x/gov/CHANGELOG.md Outdated Show resolved Hide resolved
x/gov/CHANGELOG.md Outdated Show resolved Hide resolved
x/gov/CHANGELOG.md Outdated Show resolved Hide resolved
x/gov/keeper/proposal.go Show resolved Hide resolved
x/gov/keeper/common_test.go Show resolved Hide resolved
x/gov/types/v1beta1/msgs_test.go Show resolved Hide resolved
x/gov/client/cli/prompt_test.go Show resolved Hide resolved
x/gov/types/v1/proposals_test.go Show resolved Hide resolved
x/gov/types/v1/msgs_test.go Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 0a1e101 and 322c79d.
Files selected for processing (1)
  • x/gov/keeper/proposal.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
  • x/gov/keeper/proposal.go

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 1

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 322c79d and a6d4249.
Files selected for processing (6)
  • x/gov/CHANGELOG.md (1 hunks)
  • x/gov/client/cli/prompt.go (7 hunks)
  • x/gov/client/cli/prompt_test.go (3 hunks)
  • x/gov/simulation/operations.go (7 hunks)
  • x/gov/types/v1/proposal.go (2 hunks)
  • x/gov/types/v1beta1/msgs_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (5)
  • x/gov/client/cli/prompt.go
  • x/gov/client/cli/prompt_test.go
  • x/gov/simulation/operations.go
  • x/gov/types/v1/proposal.go
  • x/gov/types/v1beta1/msgs_test.go

x/gov/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 1

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between a6d4249 and 9819366.
Files selected for processing (1)
  • x/gov/CHANGELOG.md (1 hunks)
Additional comments: 2
x/gov/CHANGELOG.md (2)
  • 66-66: The phrase "take an address.Codec as arguments" in the context of Prompt and PromptMetadata is correctly pluralized, aligning with the subject. However, ensure consistency in documentation by verifying similar constructs throughout the changelog.
  • 67-67: The correction to "takes a String as an argument" for SetProposer is accurate and improves the clarity of the documentation by specifying that a single string argument is expected instead of a fmt.Stringer interface.

x/gov/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 1

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 9819366 and 20bd143.
Files selected for processing (1)
  • x/gov/CHANGELOG.md (1 hunks)

Comment on lines +64 to +67
* [#19850](https://github.com/cosmos/cosmos-sdk/pull/19850) Removes the use of Accounts String method:
* `NewDeposit`, `NewMsgDeposit`, `NewMsgVote`, `NewMsgVoteWeighted`, `NewVote`, `NewProposal`, `NewMsgSubmitProposal` now take a string as an argument instead of an `sdk.AccAddress`.
* `Prompt` and `PromptMetadata` take an address.Codec as arguments.
* `SetProposer` takes a String as an argument instead of a `fmt.Stringer`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API Breaking Changes section lists several functions with their argument types changed from sdk.AccAddress to string. However, there are duplicate entries for NewMsgVote and NewMsgVoteWeighted. This redundancy should be corrected to maintain clarity and accuracy in the documentation.

-    * `NewDeposit`, `NewMsgDeposit`, `NewMsgVote`, `NewMsgVoteWeighted`, `NewVote`, `NewProposal`, `NewMsgSubmitProposal`, `NewMsgVote`, `NewMsgVoteWeighted` now take a string as an argument instead of an `sdk.AccAddress`.
+    * `NewDeposit`, `NewMsgDeposit`, `NewMsgVote`, `NewMsgVoteWeighted`, `NewVote`, `NewProposal`, `NewMsgSubmitProposal` now take a string as an argument instead of an `sdk.AccAddress`.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* [#19850](https://github.com/cosmos/cosmos-sdk/pull/19850) Removes the use of Accounts String method:
* `NewDeposit`, `NewMsgDeposit`, `NewMsgVote`, `NewMsgVoteWeighted`, `NewVote`, `NewProposal`, `NewMsgSubmitProposal` now take a string as an argument instead of an `sdk.AccAddress`.
* `Prompt` and `PromptMetadata` take an address.Codec as arguments.
* `SetProposer` takes a String as an argument instead of a `fmt.Stringer`.
* [#19850](https://github.com/cosmos/cosmos-sdk/pull/19850) Removes the use of Accounts String method:
* `NewDeposit`, `NewMsgDeposit`, `NewMsgVote`, `NewMsgVoteWeighted`, `NewVote`, `NewProposal`, `NewMsgSubmitProposal` now take a string as an argument instead of an `sdk.AccAddress`.
* `Prompt` and `PromptMetadata` take an address.Codec as arguments.
* `SetProposer` takes a String as an argument instead of a `fmt.Stringer`.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 20bd143 and dd9b9ef.
Files selected for processing (3)
  • x/gov/CHANGELOG.md (1 hunks)
  • x/gov/keeper/deposit.go (2 hunks)
  • x/gov/keeper/proposal.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • x/gov/keeper/deposit.go
  • x/gov/keeper/proposal.go
Additional comments: 1
x/gov/CHANGELOG.md (1)
  • 65-68: The API Breaking Changes section correctly lists the changes related to the removal of Accounts.String() method and adjustments in argument types for various functions. However, it's essential to ensure that these changes are accurately reflected in the codebase and documentation to avoid confusion.

@sontrinh16 sontrinh16 added this pull request to the merge queue Mar 26, 2024
Merged via the queue into main with commit 160c418 Mar 26, 2024
61 of 64 checks passed
@sontrinh16 sontrinh16 deleted the julian/gov-accString-removal branch March 26, 2024 10:33
meetrick pushed a commit to meetrick/cosmos-sdk that referenced this pull request Mar 28, 2024
Co-authored-by: son trinh <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants